8295851: Do not use ttyLock in BytecodeTracer::trace #25915
Conversation
|
👋 Welcome back coleenp! A progress list of the required criteria for merging this PR into |
|
@coleenp This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 81 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
| // We need a global instance to keep track of the method being printed so we can report that | ||
| // the method has changed. If this method is redefined and removed, that's ok because the method passed in won't match, and | ||
| // this will print that one. | ||
| static Method* _current_method = nullptr; |
There was a problem hiding this comment.
And if we are calling trace_interpreter from multiple threads they will each stomp on this shared global field. Sorry I can't see how some form of locking is not needed here.
There was a problem hiding this comment.
If multiple threads change the value of _current_method, the method printing will print the method name again. That's a benign race.
There was a problem hiding this comment.
So actually this wasn't more racy than what was there in this respect. You could change the global method before taking the ttyLock and for multiple threads, this would claim that the current method had changed. The old code also had shared state for BytecodeTracer. Now this uses a local instance, which can then print the bytecode at the given location.
There was a problem hiding this comment.
So the old code (mis)used a shared ByteCodeTracer instance, and the new code gives each use a local ByteCodeTracer but seeds it from the global shared field. So I agree the level of raciness seems unchanged.
But printing an unrelated method name still seems a bad thing to do. ??
BTW it is also very confusing to give the global variable the same name as the private field in ByteCodeTracer.
There was a problem hiding this comment.
It would print the right name it would just print it again. Say:
t1 printing M1 so prints the name
t1 aload
t1 astore
t2 printing M2 method name
t2 getfield
t1 < continues printing M1 but method name was changed to M2> prints M1 method name
t1 invokevirtual
...
The thread id is printed before each line. Using this with multiple threads is not ideal for getting information clearly but that's not what people do.
| // There used to be a leaf mutex here, but the ttyLocker will | ||
| // work just as well, as long as the printing operations never block. | ||
| _interpreter_printer.trace(method, bcp, tos, tos2, st); | ||
| BytecodePrinter printer(_current_method); |
There was a problem hiding this comment.
You need a Atomic::load_acquire here to pair with the Atomic::release_store below.
There was a problem hiding this comment.
ok, added and changed the name of current_method, and added comments. Hope it helps.
Oh yes that would be better, but I hate to add things to Thread for just one use. Actually, reprinting the method name probably helps with readability if multiple threads are interleaving output. Thanks for the review @dholmes-ora |
This didn't need ttyLock for synchronization, the code only needs to see if the method changes so it can print the method name before the bytecodes, like:
[490166] static void java.lang.String.()
[490166] 13 18 putstatic 613 <java/lang/String.CASE_INSENSITIVE_ORDER:Ljava/util/Comparator;>
[490166] 14 21 return
[490166] static void java.lang.System.()
[490166] 15 0 invokestatic 471 <java/lang/System.registerNatives()V>
[490166] 16 3 aconst_null
[490166] 17 4 putstatic 474 <java/lang/System.in:Ljava/io/InputStream;>
Verified manually and added some parallelism to the test, and fixed trace() to initialize is_linked(), which it always is.
Also ran tier1-4.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25915/head:pull/25915$ git checkout pull/25915Update a local copy of the PR:
$ git checkout pull/25915$ git pull https://git.openjdk.org/jdk.git pull/25915/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25915View PR using the GUI difftool:
$ git pr show -t 25915Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25915.diff
Using Webrev
Link to Webrev Comment